-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cube writer to properly write out cube generated by model fitting #2012
Add cube writer to properly write out cube generated by model fitting #2012
Conversation
Coming from the other thread: this lgtm, but I'm not giving a formal review approval since I'm not part of the organization. I just don't see any obvious problems with this approach. I'll note that this effectively establishes a standard in which the 'MASK' HDU stores the mask, which isn't necessarily widely adopted. I like it and can immediately add it to the spectral-cube FITS reader. Could you add to the docs, though, where the rules for the MASK are? I think it should be listed in the docstring of the writer. I'm pretty sure 0 means 'good', and anything nonzero is some form of 'bad', but past that I think it is in the JWST docs somewhere? |
Good question! I didn't even consider that the mask might be flipped because it is using the |
This is also a good point. I picked MASK from this existing code but that happened before I joined the project, so I would also need to consult with other devs.
Masking was discussed in APE 11 that never got merged and also that discussion was not really for spectra, though you would think masking would be general enough to also cover it. In real HST and JWST data, they actually contained bit-plane mask, not just simple 0s and 1s. AFAIK, that is currently not supported by Jdaviz parsers. Regardless, naively, I would assume 0 means "good" because it is defined as such for the bit-plane stuff. |
0 PRIMARY 1 PrimaryHDU | ||
1 SCI 1 ImageHDU (float32) | ||
2 MASK 1 ImageHDU (uint16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the file structure you need, I don't think wcs1d-fits
can do it in its present form – with or without the new PR. You can write to the first extension instead of the primary with write(..., hdu=1, format='wcs1d-fits')
, but currently there is no provision to write the mask. It could certainly be added, but last I recall specutils had rather poor support to write to a new HDU in an existing HDUList.
If this loader is working as it should, might be best to use it as is and perhaps then contribute to the default_loaders
upstream. Would this make sense to provide as a writer for jwst_s3d_loader
, or is that yet another format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has nothing to do with JWST, it is a "generic" format for a cube output from model fitting plugin that could or could not have mask populated. So putting this in jwst_s3d_loader
would be very misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it would probably belong into wcs1d-fits
. I have not found much on mask treatment apart from SDSS/MaNGA (which is storing it in hdulist['MASK']
), so otherwise don't know what would be a standard for storing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno either. If this is not generic enough, personally, I am okay with this writer just living in Jdaviz forever.
996a5c8
to
f664ab3
Compare
Codecov ReportBase: 92.06% // Head: 92.09% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2012 +/- ##
==========================================
+ Coverage 92.06% 92.09% +0.02%
==========================================
Files 140 140
Lines 15305 15373 +68
==========================================
+ Hits 14091 14158 +67
- Misses 1214 1215 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
The ASDF warnings in dev job are unrelated. ASDF is changing upstream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this seems like a reasonable approach to me for now. What are other valid format names (from upstream)? Would it make more sense to be just "jdaviz-cube" since that describes the format of the data (rather than "-writer" which to me more describes the method to export the data)?
jdaviz/configs/cubeviz/helper.py
Outdated
Examples | ||
-------- | ||
To write out a Spectrum1D cube using this writer: | ||
|
||
>>> spec.write("my_output.fits", format="jdaviz-cube-writer", overwrite=True) # doctest: +SKIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be an example on how to re-load the resulting fits file back into a Spectrum1D or is that just considered obvious? Does that require providing the format again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just use vanilla Spectrum1D.read
. Existing Cubeviz parser can read it back.
This comment was marked as outdated.
This comment was marked as outdated.
Is that true? It looks like other cases use the same
Should this be covered in the test? The test currently just loads with As a user, I would just expect the same string to be required (or at least to work) when writing and then reading, since the kwargs is format and not writer. So even if it isn't required, does |
Actually now that I think about it, if this really follows the |
Sure, doesn't hurt. The reader would raise error because that format isn't register on the reader side. I can also add a test for that. |
f664ab3
to
b70abeb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Yes, the typical pattern in the default loaders is to provide both a |
out cube generated by model fitting.
[ci skip]
b70abeb
to
ad0d3e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall!
I agree that we should be careful to write down somewhere that we're sticking with these mask conventions, as mentioned in #2012 (comment). I agree that zero/False should correspond to "unmasked," and non-zero/True to "masked."
jdaviz/configs/default/plugins/model_fitting/tests/test_fitting.py
Outdated
Show resolved
Hide resolved
Is the additional text linking to specutils doc in the diff here not enough, @bmorris3 ? |
I expect users will tell us if this is confusing. I doubt most of our users are even aware that glue masks are the inverse of this convention, so it's probably not a top priority to inject that confusion into our docs. In summary – I don't think we should worry about it. 💯 |
I think I addressed all the comments? Can I merge when CI pass since there are 2 approvals or is there something else? Thanks! |
Thanks, all! |
Description
This pull request is to add a custom specutils writer so one can write out spectral cube (as Spectrum1D object) to a FITS file, and then have it read back in as a cube (not a table).
This is necessary until the following issue/PR are resolved upstream:
wcs1d-fits
loader for multi-D WCS astropy/specutils#1009With this patch, you can do cube fitting, put the fitted cube in uncert-viewer, and then also do this:
Then you can read it back into Cubeviz (a new instance) like this:
TODO
0=good
or0=bad
? We want0=good
.Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.